-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for defaults #44
Conversation
function defaultLoan(address loan) | ||
external | ||
onlyManager | ||
atState(IPoolLifeCycleState.Active) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely correct, but I was torn whether we want it to be enforced, or leave it to the PM's discretion if something gets into a weird state (only way that it could would be a bug, but still).
@@ -219,13 +224,31 @@ contract Pool is IPool, ERC20 { | |||
function fundLoan(address addr) external onlyManager { | |||
ILoan loan = ILoan(addr); | |||
loan.fund(); | |||
_accountings.activeLoanPrincipals += loan.principal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be overwritten by the loan funding work, but the accounting in the default overflows without it, so I added it in
/** | ||
* @dev Mapping of created loans | ||
*/ | ||
mapping(address => bool) private _isLoan; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calling this one out -- I can see an argument for this to instead live on the ServiceConfiguration, perhaps in a larger mapping keyed off of the loanFactory address.
This would map loanFactoryAddress => createdLoanAddress => bool
, or something similar.
This might be slightly more testable, but perhaps overloads the Config contract. Curious if anyone has thoughts!
/** | ||
* @dev Holds a reference to valid LoanFactories | ||
*/ | ||
mapping(address => bool) public isLoanFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment re: LoanFactory mapping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes:
defaultLoan
on the Pool, callable by the PM only while the pool isActive
activeLoanPrincipals
according to the defaulted loan principal